Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix tools build #6475

Merged
merged 12 commits into from
Sep 8, 2023
Merged

Fix tools build #6475

merged 12 commits into from
Sep 8, 2023

Conversation

kiburtse
Copy link
Contributor

@kiburtse kiburtse commented Apr 5, 2023 •
edited
Loading

What, How & Why?

☑️ ToDos

  • 📝 Changelog update
  • [ ] 🚦 Tests (or not relevant)
  • [ ] C-API, if public C++ API changed.

@kiburtse kiburtse requested a review from ironage April 5, 2023 21:30
@cla-bot cla-bot bot added the cla: yes label Apr 5, 2023
ironage
ironage approved these changes Apr 5, 2023
View reviewed changes
Copy link
Contributor

@ironage ironage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once CI is passing 👍

src/realm/exec/CMakeLists.txt Outdated
add_custom_target(build_all DEPENDS
RealmImporter RealmTrawler RealmEnumerate RealmDecrypt
RealmEncrypt RealmBrowser RealmDump)
add_dependencies(build_all ${ExecTargetsToInstall})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

CMake Error at src/realm/exec/CMakeLists.txt:75 (add_dependencies):
  add_dependencies called with incorrect number of arguments

I think you might want to disable building these tools on Android

@kiburtse
Copy link
Contributor Author

kiburtse commented Apr 6, 2023

would the change log entry be needed for such internal thing?

@ironage
Copy link
Contributor

ironage commented Apr 6, 2023

I think excluding these tools from the default build was intentional to save compile time. (the downside as you discovered is that they can easily bitrot). Instead of making them part of the default build, how about explicitly building them on one select CI machine?

would the change log entry be needed for such internal thing?

Sure, you can add a note in the internals section of the change log.

@kiburtse kiburtse force-pushed the kb/fix-tools-build branch from 87bcddc to 5e83928 Compare May 10, 2023 16:26
@kiburtse kiburtse marked this pull request as draft May 12, 2023 22:34
@kiburtse kiburtse force-pushed the kb/fix-tools-build branch 2 times, most recently from aac0ee0 to 733c298 Compare September 7, 2023 10:07
@kiburtse kiburtse marked this pull request as ready for review September 7, 2023 12:27
@kiburtse
Copy link
Contributor Author

kiburtse commented Sep 8, 2023

I think excluding these tools from the default build was intentional to save compile time. (the downside as you discovered is that they can easily bitrot). Instead of making them part of the default build, how about explicitly building them on one select CI machine?

would the change log entry be needed for such internal thing?

Sure, you can add a note in the internals section of the change log.

I've excluded a few configurations, but it's less than one second on a laptop, practically it saves not much, i think it should be fine.

@kiburtse kiburtse merged commit 75cc43e into master Sep 8, 2023
@kiburtse kiburtse deleted the kb/fix-tools-build branch September 8, 2023 09:04
@kiburtse kiburtse mentioned this pull request Sep 8, 2023
Merged
3 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ironage ironage ironage approved these changes

Assignees

@kiburtse kiburtse

Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kiburtse @ironage